Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fuzzer: Use a directory for important fuzz testcases #6297

Merged
merged 7 commits into from
Feb 12, 2024

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 10, 2024

This removes the manual list inside the fuzzer with a simple directory that we
scan. The manual list was not well-maintained and nothing there really
mattered enough to keep. Instead, users can place files in /test/fuzz/ that they
consider important.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable, although one thing to note is that that may make reproducibility slightly worse since the fuzz results will now depend on the contents of the fuzz directory.

@@ -1402,6 +1388,8 @@ def handle(self, wasm):
lit_tests = shared.get_tests(shared.get_test_dir('lit'), test_suffixes, recursive=True)
all_tests = core_tests + passes_tests + spec_tests + wasm2js_tests + lld_tests + unit_tests + lit_tests

fuzz_cases = shared.get_tests(shared.get_test_dir('fuzz'), test_suffixes, recursive=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't break if the fuzz directory doesn't exist, will it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add a readme.txt in that directory to force it to exist in git.

@aheejin
Copy link
Member

aheejin commented Feb 10, 2024

I'm not strongly opinionated, but wouldn't it be simpler test directories are categorized based on their kinds (basic, passes, binary, ...) and not their temporary fuzz importances?

@kripken
Copy link
Member Author

kripken commented Feb 12, 2024

@aheejin

I'm not strongly opinionated, but wouldn't it be simpler test directories are categorized based on their kinds (basic, passes, binary, ...) and not their temporary fuzz importances?

I'll clarify in a comment more what I intend here, I think I wasn't clear enough. The new directory is for manually adding new fuzz testcases for the fuzzer - we wouldn't move any existing testcases there, and probably would not even commit anything there. But when fuzzing locally the user can drop a few files in there that get coverage that way.

@aheejin
Copy link
Member

aheejin commented Feb 12, 2024

That makes sense. Thanks for the explanation!

@kripken
Copy link
Member Author

kripken commented Feb 12, 2024

After thinking a little I think maybe it is clearer to make the directory not exist and document the feature as being for local changes, that is, now the docs say "If you want to fuzz some files with high importance, create 'fuzz' and put the files there". That's the intended use case so we might as well describe it that way.

@tlively
Copy link
Member

tlively commented Feb 12, 2024

The advantage of checking in the directory with a readme is that it makes this feature and its documentation more discoverable, but either way sgtm.

@kripken
Copy link
Member Author

kripken commented Feb 12, 2024

Makes sense, maybe the important thing is to keep the fuzz dir parallel to the test dir, so it's clear normal testcases do not go there. I did that now and clarified in the readme there.

@kripken
Copy link
Member Author

kripken commented Feb 12, 2024

@kripken kripken merged commit 88fe1b6 into WebAssembly:main Feb 12, 2024
14 checks passed
@kripken kripken deleted the fuzz.cases branch February 12, 2024 23:02
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants